-
Notifications
You must be signed in to change notification settings - Fork 187
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Allow additional handlers for special selectors to be added. #95
Conversation
* example above. | ||
* @param {string} baseSelector: The selector of the parent styles. | ||
* '.foo:nth-child(2n)' in the example above. | ||
* @param {function} callback: A function which can be called to generate CSS |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we call callback
something else? When I see the word callback
I think: "Call this thing when you're done." Maybe that's just me, but I think a more descriptive name would be appropriate.
Not sure what's best, but maybe: generateSubtreeStyles
?
From a user perspective, it matters less in this code, but what we call it in the docs will be an important way to communicate what it's for.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
generateSubtreeStyles
indeed sounds much better.
This is an interesting idea. Unless I'm missing something, this would allow for a plugin ecosystem to be developed around aphrodite which would be neat. |
You aren't missing something, that's the idea! |
Though, at the same time, I'd like to figure out a way to ensure that the dependency a given set of styles has on such a plugin were more explicit... Maybe forcing that plugin be provided to any call into |
I thought about that too. The main problem I found with it is that these plugins are actually going to be used at Also we currently cache and hash the styles based only on the classname, so if you tried to generate the same styles based on different plugins it wouldn't do anything? |
True, but what's less painful is if this can be curried in some fassion, like so: const myCss = css(myCustomExtensions)
myCss(styles.bar, styles.foo) Then people could pass Apologies that I haven't been helpful with actual code in aphrodite. I haven't had the chance to dive into the code much. Hopefully my feedback is helpful as a user though! |
This is an interesting idea! As far as I can tell, if someone were to implement the But the idea of allowing for plugins, thereby allowing Aphrodite's core to stay nice and small appeals to me. I would really love Aphrodite to be the variety of project that can be called "done" and stop development on it, leaving it in a healthy, working state. As for the dependency issue, here's my suggestion, which also addresses my concern about the "no way to undo this". // aphrodite-with-extensions.js
import {StyleSheet, css} from 'aphrodite';
export default StyleSheet.extend([customSelectorHandler1, customSelectorHandler2]); Then to use the version with extension, you use import {StyleSheet, css} from './aphrodite-with-extensions.js' Where |
I love the |
One other thing to support the idea of not changing the default handlers from aphrodite globally is if I am using aphrodite in my app and two of my dependencies are using aphrodite, they could potentially muck with each other's handlers. Or, even more likely, they could define handlers that conflict with one another. Either way, this would be highly unexpected, so doing something like @jlfwong is suggesting would be fantastic. |
I love the (maybe |
bdb3791
to
801d53d
Compare
Okay, I finally got around to updating this pull request. Getting the Also, reminder to myself that I should write something in the readme about this before it lands. |
}); | ||
|
||
it('uses a new selector handler', done => { | ||
const descendantHandler = (selector, baseSelector, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What would these arguments look like in this situation:
{
foo: {
'^bar': {
'^baz': {
}
}
}
}
Just trying to think of edge cases to make sure that the plugin system makes sense.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's try it! I edited the test to be
const sheet = newStyleSheet.create({
foo: {
'^bar': {
'^baz': {
color: 'orange',
},
color: 'red',
},
color: 'blue',
},
});
and it generates:
.foo_1a51bwt {
color:blue !important;
}
.bar .foo_1a51bwt {
color:red !important;
}
.baz .bar .foo_1a51bwt {
color:orange !important;
}
Looks like what we'd expect?
I'm not certain that I really understand all the code behind this (I've still yet to really take the time to learn the codebase), but as long as the API it exposes is flexible enough to support a good plugin system then I say 👍 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice! I really like how this turned out.
This is going to need new documentation both on how to use the feature and with some warnings about how you can shoot yourself in the foot with this feature, since you'll be able to create syntax extensions with non-deterministic behavior due to injection order.
This also could be used to allow people to generate global styles, right?
function globalStyles(selector, baseSelector, callback) {
if (selector[0] !== '!') {
return null;
}
return callback(selector.slice(1));
};
function descendant(selector, baseSelector, callback) {
if (selector[0] !== '^') {
return null;
}
return callback(`.${selector.slice(1)} ${baseSelector}`);
};
Would love to get the docs knocked out as part of this diff too, since documenting it might reveal certain bits of non-obvious awkwardness.
I'm a little worried about how much firepower this gives for people to shoot themselves in the foot. People are already doing that with the StyleSheet.create({foo: {':hover .child': {...}})
hack anyway, but I'm a bit concerned that this might seem like it's providing standardized foot gunpowder.
This also opens people up to do dangerous things in their selectors like generating non-deterministic selectors by using global information (like feature detection).
I think all of these things are fine so long as we're careful to preface the "Advanced: Extensions" documentation with a scary warning about what can go wrong.
* Accepts a new baseSelector to use for generating those styles. | ||
* @returns {?string} The generated CSS for this selector, or null if we don't | ||
* handle this selector. | ||
*/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Amazing docs 😍
// how our selector handlers work, instead of passing in | ||
// `selectorHandlers` and have them make calls to `generateCSS` | ||
// themselves. Right now, this is impractical because our string | ||
// handlers are very specialized and do complex things. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reasonable middle ground here using partial application to avoid needing stringHandler functions to know anything about selectorHandlers
? I guess it's a little awkward because the function signature ends up being really similar to generateCSS
, but not quite?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess I mostly wrote this TODO because I think this would be another pretty easy place to hook up another extension bit, but it isn't ready for that yet because our current versions use a bunch of internal functions and state.
Is there a reasonable middle ground here using partial application to avoid needing stringHandler functions to know anything about selectorHandlers?
This is what I was trying to convey in this comment, you did it much better than I did. :)
* | ||
* @param {Object} extensions: An object containing the extensions | ||
* to add to the new instance of Aphrodite. | ||
* @param {Array.<SelectorHandler>} extensions.selectorHandlers: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this how you document this? To me, this jsdoc reads like extend
takes 2 args.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is how http://usejsdoc.org/tags-param.html#parameters-with-properties says to do it!
Yes to all of that! We're definitely going to have to put some warning labels up around this, so if things don't work then they know it's because the extensions can do buggy things, and not because Aphrodite has problems.
The last step of this diff is to write the docs, I just didn't want to do that if we were going to change everything around again. :)
I believe so! (ping #139) Your example would be a little awkward in the styles, though: StyleSheet.create({
'!#blah': {},
'!body': {},
'!.my-class': {},
}); I guess we can bikeshed what that extension would look like later. :P (other random question: if we made "official" extensions, would we want them to live in this repo or would we make separate repos for them?) |
Yep, API looks good to me. Write them docs! For the global styling, it would be:
Then injection would happen with
Would prefer they live in other repositories, or for "official" extensions to just straight up not exist :) |
Agreed 👍 that would make the most sense. If a large community of extensions is developed around aphrodite, perhaps an aphrodite org with "official" extensions could reside in that org. But that's not something to think about right now I'd say. |
Okay, I added some docs, and changed the API a little bit to make it so people using the extensions don't need to know what kind of extension it is. Let me know how the docs look! (and how the API looks?) Maybe now that we have so many jsdocs I should figure out how to actually turn those into something people can look at... |
You might try https://doclets.io/ |
@kentcdodds Interesting! https://doclets.io/Khan/aphrodite/add-to-doclets Pretty easy to set up :) Also, looks like maybe our comments aren't formatted correctly? Some of the docs there look weird like the return value of https://doclets.io/Khan/aphrodite/add-to-doclets#dl-generateCSSRuleset which is probably our fault. |
Plain Aphrodite, when used properly, ensures that the correct styles will | ||
always be applied to elements. Due to CSS specificity rules, extensions might | ||
allow you to generate styles that conflict with each other, causing incorrect | ||
styles to be shown. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this could be aided a lot by an example after the section on Creating extensions so extension authors know exactly the kind of trouble they can run into.
OH BOY, i love this, it's waaay better than my quick n dirty #148. I do think Aphrodite greatly needs the ability for globals. And I can certainly see the benefit for nested selectors. That gives us 4 desired features:
Correct me if I'm wrong, but this PR would solve 1 & 2. The door would be left open for solving 3 & 4, but as mentioned, the API would require proprietary prefixes like ! and ^ to achieve that. Certainly possible, and hey, maybe we could create a standard for what those prefixes mean in JS land. However, I'd argue that by adding an extra function to the API, say |
@jlfwong I added an example of generating an extension so the warnings are clearer. What do you think? |
@mattkrick Since adding extensions is isolated from the rest of aphrodite, you could somewhat trivially implement import StyleSheet from "aphrodite";
const myGlobalExtension = {
selectorHandler: (selector, _, generateSubtreeStyles) => {
if (selector[0] !== "*") {
return null;
}
return generateSubtreeStyles(selector.slice(1));
}
};
const {css, ExtendedStyleSheet} = StyleSheet.extend([myGlobalExtension]);
export default function cssGlobal(globalStyles) {
const styles = {};
Object.keys(globalStyles).forEach(key => {
styles['*' + key] = globalStyles[key];
});
css(StyleSheet.create({
global: styles,
}).global);
} and then use it like import cssGlobal from 'css-global.js';
cssGlobal({
'body': {
margin: 0,
}
'a': {
textDecoration: 'none',
':visited': {
color: 'blue',
},
},
}); |
Random thoughts: I'm now realizing that in this implementation, the So for instance, if I do something like
then my extended things aren't going to be handed, right? I'm not sure what an appropriate way to reconcile these things is. 🙁 I'll think about this later... |
We added a new function locally to create a scoped css rules. The idea is you call export const globalCssClass =
(scope: string, sheetDefinition: StyleSheetDefinition<*>): string => {
const hashedScopeName =
process.env.NODE_ENV === 'test'
? scope
: `${scope}_${hashObject(sheetDefinition)}`
const styleSheets = map(sheetDefinition, (_definition, name) => ({
_name: `${hashedScopeName} ${name}`,
_definition,
}))
styleSheets.map(sheet => css(sheet))
return hashedScopeName
} |
@@ -292,11 +292,51 @@ extension that you created, and pass that into `StyleSheet.extend()`: | |||
```js | |||
const mySelectorHandler = ...; | |||
|
|||
const myExtension = { selectorHandler: mySelectorHandler }; | |||
const myExtension = {selectorHandler: mySelectorHandler}; | |||
|
|||
StyleSheet.extend([myExtension]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This line wouldn't do anything, right? It's only the return value of this that would contain the extension?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah yes, that is correct. I'm not sure how to indicate that. module.exports =
? const {StyleSheet, css} =
?
css(styles2.globals); | ||
``` | ||
|
||
It isn't determinate whether divs will be red or blue. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hrm, I'm not sure I'd say it's non deterministic -- I think this falls into what compilers typically refer to as "undefined behavior", right? So we can say that it's undefined behavior whether the divs will be red or blue.
Given this precise code listed here, the result will be deterministic, no?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes that is correct. "Undefined behaviour" sounds better.
@aaronjensen as an FYI, the code you have there uses Aphrodite internals and is not guaranteed to continue working in future versions of Aphrodite (the @mattkrick It's not just about removing extensions, it's about conflicts. Imagine that waaaay down in your stack, in a component that's used in a component that you didn't write, happens to use Aphrodite, and it happens to use a plugin that wants to use the same syntax as a different plugin that you've written. You really, really don't want those two instances of Aphrodite interacting. @xymostech I think that case is fine. As much as possible, I'd really like things that use Aphrodite to be doing so as an implementation detail, and not expose that fact to the world, so styles being passed between two different instances of Aphrodite should be a rare occurrence. If we really wanted to enforce this, we could tag styles created with This diff now LGTM, but it needs conflict resolving. Excited to get this landed! |
Okay. I'm still a little hesitant, but I guess if people start complaining we can re-evaluate. |
@xymostech If you can think of a reasonable way to have the transformation take place at |
eaa523f
to
bd9407b
Compare
Summary: This adds the ability for users of Aphrodite to add extensions which support other special selectors. This should let people add extensions to write global styles or many other not-terribly-well-supported use cases until we have better solutions. This adds docs on how to use the extensions, and adds a bunch of jsdocs to make things clear in the code. Test Plan: - `npm run test`
bd9407b
to
6eaf4f5
Compare
Okay, rebased and squashed! I'll merge and release a new version. |
Is there documentation for this yet? I think I need to build a plugin to support RTL (like https://github.com/cssjanus/cssjanus does) |
@kentcdodds Is https://github.com/Khan/aphrodite#advanced-extensions enough to get started? I actually don't know if our extension API is sufficiently powerful to do automatic RTL conversion though. |
Ah, somehow missed that. Thanks @jlfwong! I just realized... This appears to only support modifying selectors, not the styles themselves. :-( So I don't think that supporting RTL transformation is possible at the moment :-( |
@kentcdodds Well, you get the CSS string back from |
Yeah, also it probably wouldn't perform well. I'd rather just manipulate the objects themselves before I pass them to aphrodite. I think that I'm going to do that. |
@kentcdodds Ah, yeah! That's probably a good approach. Something like this? const styles = StyleSheet.create(rtlConvert({
something: {
textAlign: 'left'
},
})); |
Yep, I wish that something like that existed already, but I don't think it does :-( |
@kentcdodds i'm guessing your usecase is when someone switches a language? In that case, you can just treat it like a theme. You have your theme/language passed in via context, you memoize on the context in a |
@kentcdodds Really looking forward to seeing that! Maybe something like that "traverse-and-modify" will become a more common tool. Maybe the transform could even be done using a babel plugin, so it's not done on the fly? @mattkrick You could treat it like a theme, but the point of doing an rtl conversion is that you can automatically figure out what the new styles will be based on the old styles. I think what you're suggesting would require re-writing all of the styles for both ltr and rtl instead of automatically converting. |
I started it here: https://github.com/kentcdodds/rtl-css-js Everything's ready to start adding tests and making them work. Check the issues :) |
Are there any extensions out there already? Links to examples in the docs would be reassuring to people who are looking. And maybe suggest a standard naming convention to make it easier to find |
What's the recommended way to do descendant selectors? Is it just the already existing (but unintentional) |
@jlfwong @kentcdodds @xymostech Hi, What's the recommended way to do descendant selectors? E.g. |
@Baggz I think https://github.com/Khan/aphrodite#advanced-extensions contains example code for one way to tackle that. I'm not aware of a canonical solution, however. |
Summary: This adds the ability for users of Aphrodite to add extensions
which support other special selectors. I'm not sure if the interface
that is exposed is expressive enough to allow people to make all of the
changes that they might want to, but it would let people do the
selector(...)
thing:Maybe fixes #10
Test Plan:
npm run test
@jlfwong @zgotsch @kentcdodds @montemishkin